Move header value normalisation into Response #81
Conversation
Only concern is whether we are breaking any pre-existing behaviour. I suggest:
|
| @@ -47,10 +46,12 @@ public function get($uri, array $headers = []) | |||
| } | |||
| $response = $this->client->send(); | |||
|
|
|||
| $headers = $response->getHeaders()->toArray() ?? []; | |||
There was a problem hiding this comment.
Im theory this is always an array?
There was a problem hiding this comment.
Eh yes, forgot the ?->, will fix it. getHeaders is returning null|Headers
There was a problem hiding this comment.
Oh I see what you mean now. AbstractMessage (Laminas\Http) handles null. IDE was showing it null|Headers for whatever reason. null is handled on getHeaders.
| @@ -155,14 +157,16 @@ private function validateHeaders(array $headers) | |||
|
|
|||
| /** | |||
| * Normalize header names to lowercase. | |||
| * Also ensures multi-value headers are represented as a single string, via | |||
| * comma concatenation. | |||
| * | |||
| * @return array | |||
There was a problem hiding this comment.
Let's document this to be an array<string> then? Previously, it was an array<string|array<string>>, so this change looks like a breaking change 🤔
There was a problem hiding this comment.
Its the opposite I think. Before the normalizeHeaders was expecting array<string, string> and now it can handle array<string, string|array<string>>, as the logic that was in LaminasHttpClientDecorator prepareResponseHeaders now is included in Response normalizeHeaders (see validateHeaders in Response is not accepting multi value (L147 in my branch).
So I thought its not a breaking change as now it can handle what it was handling before and in addition multi-value headers?
Yes, will check more myself for BC breaks and do the suggestions soon. |
Signed-off-by: thgs <theogpl57@gmail.com>
thgs
force-pushed
the
move-header-normalisation-into-response
branch
from
dc1f18a
to
63614b8
Compare
Signed-off-by: thgs <theogpl57@gmail.com>
thgs
force-pushed
the
move-header-normalisation-into-response
branch
from
63614b8
to
f928367
Compare
Signed-off-by: thgs <theogpl57@gmail.com>
|
Happy new year. I think I am going to back off from this for now. The benefit is very little and it seems more risky the more I think about it. Thanks for review! |
Description
When not using laminas-http as the HTTP client the implementation of the
HeaderAwareClientInterfaceone may have to reproduce the logic to normalise headers from multi value to string as it is done in here.I am partial about whether what I am doing here is right (or desired) or wrong (will have problems), so it is more of a question, in the form of a PR for clarity.
The benefit of this change is that when implementing the interfaces needed to inject a third party client, one does not need to deal with the case of multi value headers and can reuse the Response class provided (
Laminas\Feed\Reader\Http\Response), therefore only one implementation is needed implementingHeaderAwareClientInterface.I am relatively new to the codebase so let me know if I have missed some profound/foundational concern here. I can see a slight violation of single responsibility principle here.
On the other hand, it is a (micro?) optimisation as the loop through the headers only happens once.
Lastly, given this is desired, maybe a unit test case should be added as well as an addition in the docs explaining the above "convenience". Ofc I am willing to do these and any other changes requested but I thought it would be nice if first it is acknowledged that the direction is desired.